Be careful with page_get_owner() now that owner field can be clobbered

by some users. Introduce get_page_owner_and_reference() where that can
be more useful.

Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index cbbe2e0abc6311802c74b021dc885e665bb8229b..006964498f6d684ac4b4cb8e45e67a0799454294 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -371,14 +371,14 @@ void share_xen_page_with_privileged_guests(
 #else
 /*
  * In debug builds we shadow a selection of <4GB PDPTs to exercise code paths.
- * We cannot safely shadow the idle page table, nor shadow (v1) page tables
- * (detected by lack of an owning domain). As required for correctness, we
+ * We cannot safely shadow the idle page table, nor shadow page tables
+ * (detected by zero reference count). As required for correctness, we
  * always shadow PDPTs above 4GB.
  */
-#define l3tab_needs_shadow(mfn)                         \
-    (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) && \
-      (page_get_owner(mfn_to_page(mfn)) != NULL) &&     \
-      ((mfn) & 1)) || /* odd MFNs are shadowed */       \
+#define l3tab_needs_shadow(mfn)                          \
+    (((((mfn) << PAGE_SHIFT) != __pa(idle_pg_table)) &&  \
+      (mfn_to_page(mfn)->count_info & PGC_count_mask) && \
+      ((mfn) & 1)) || /* odd MFNs are shadowed */        \
      ((mfn) >= 0x100000))
 #endif
 
@@ -690,7 +690,16 @@ get_##level##_linear_pagetable(                                             \
 
 int is_iomem_page(unsigned long mfn)
 {
-    return (!mfn_valid(mfn) || (page_get_owner(mfn_to_page(mfn)) == dom_io));
+    struct page_info *page;
+
+    if ( !mfn_valid(mfn) )
+        return 1;
+
+    /* Caller must know that it is an iomem page, or a reference is held. */
+    page = mfn_to_page(mfn);
+    ASSERT((page->count_info & PGC_count_mask) != 0);
+
+    return (page_get_owner(page) == dom_io);
 }
 
 
@@ -703,7 +712,6 @@ get_page_from_l1e(
     uint32_t l1f = l1e_get_flags(l1e);
     struct vcpu *curr = current;
     struct domain *owner;
-    int okay;
 
     if ( !(l1f & _PAGE_PRESENT) )
         return 1;
@@ -714,8 +722,13 @@ get_page_from_l1e(
         return 0;
     }
 
-    if ( is_iomem_page(mfn) )
+    if ( !mfn_valid(mfn) ||
+         (owner = page_get_owner_and_reference(page)) == dom_io )
     {
+        /* Only needed the reference to confirm dom_io ownership. */
+        if ( mfn_valid(mfn) )
+            put_page(page);
+
         /* DOMID_IO reverts to caller for privilege checks. */
         if ( d == dom_io )
             d = curr->domain;
@@ -731,33 +744,29 @@ get_page_from_l1e(
         return 1;
     }
 
+    if ( owner == NULL )
+        goto could_not_pin;
+
     /*
      * Let privileged domains transfer the right to map their target
      * domain's pages. This is used to allow stub-domain pvfb export to dom0,
      * until pvfb supports granted mappings. At that time this minor hack
      * can go away.
      */
-    owner = page_get_owner(page);
-    if ( unlikely(d != owner) && (owner != NULL) &&
-         (d != curr->domain) && IS_PRIV_FOR(d, owner) )
+    if ( unlikely(d != owner) && (d != curr->domain) && IS_PRIV_FOR(d, owner) )
         d = owner;
 
     /* Foreign mappings into guests in shadow external mode don't
      * contribute to writeable mapping refcounts.  (This allows the
      * qemu-dm helper process in dom0 to map the domain's memory without
      * messing up the count of "real" writable mappings.) */
-    okay = get_data_page(
-        page, d,
-        (l1f & _PAGE_RW) && !(paging_mode_external(d) && (d != curr->domain)));
-    if ( !okay )
-    {
-        MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte
-                " for dom%d",
-                mfn, get_gpfn_from_mfn(mfn),
-                l1e_get_intpte(l1e), d->domain_id);
-    }
-    else if ( pte_flags_to_cacheattr(l1f) !=
-              ((page->count_info >> PGC_cacheattr_base) & 7) )
+    if ( (l1f & _PAGE_RW) &&
+         !(paging_mode_external(d) && (d != curr->domain)) &&
+         !get_page_type(page, PGT_writable_page) )
+        goto could_not_pin;
+
+    if ( pte_flags_to_cacheattr(l1f) !=
+         ((page->count_info >> PGC_cacheattr_base) & 7) )
     {
         unsigned long x, nx, y = page->count_info;
         unsigned long cacheattr = pte_flags_to_cacheattr(l1f);
@@ -786,7 +795,16 @@ get_page_from_l1e(
 #endif
     }
 
-    return okay;
+    return 1;
+
+ could_not_pin:
+    MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte
+            " for dom%d",
+            mfn, get_gpfn_from_mfn(mfn),
+            l1e_get_intpte(l1e), d->domain_id);
+    if ( owner != NULL )
+        put_page(page);
+    return 0;
 }
 
 
@@ -1174,7 +1192,7 @@ static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
     for ( i = 0; i < PDPT_L2_ENTRIES; i++ )
     {
         l2e = l2e_from_page(
-            virt_to_page(page_get_owner(page)->arch.mm_perdomain_pt) + i,
+            virt_to_page(d->arch.mm_perdomain_pt) + i,
             __PAGE_HYPERVISOR);
         l2e_write(&pl2e[l2_table_offset(PERDOMAIN_VIRT_START) + i], l2e);
     }
@@ -1924,7 +1942,7 @@ void put_page(struct page_info *page)
 }
 
 
-int get_page(struct page_info *page, struct domain *domain)
+struct domain *page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
 
@@ -1933,22 +1951,29 @@ int get_page(struct page_info *page, struct domain *domain)
         if ( unlikely((x & PGC_count_mask) == 0) ||  /* Not allocated? */
              /* Keep one spare reference to be acquired by get_page_light(). */
              unlikely(((x + 2) & PGC_count_mask) <= 1) ) /* Overflow? */
-            goto fail;
+            return NULL;
     }
     while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
 
-    if ( likely(page_get_owner(page) == domain) )
+    return page_get_owner(page);
+}
+
+
+int get_page(struct page_info *page, struct domain *domain)
+{
+    struct domain *owner = page_get_owner_and_reference(page);
+
+    if ( likely(owner == domain) )
         return 1;
 
     put_page(page);
 
- fail:
     if ( !_shadow_mode_refcounts(domain) && !domain->is_dying )
         gdprintk(XENLOG_INFO,
                  "Error pfn %lx: rd=%p, od=%p, caf=%08lx, taf=%"
                  PRtype_info "\n",
-                 page_to_mfn(page), domain, page_get_owner(page),
-                 y, page->u.inuse.type_info);
+                 page_to_mfn(page), domain, owner,
+                 page->count_info, page->u.inuse.type_info);
     return 0;
 }
 
@@ -1974,7 +1999,6 @@ static void get_page_light(struct page_info *page)
     while ( unlikely(y != x) );
 }
 
-
 static int alloc_page_type(struct page_info *page, unsigned long type,
                            int preemptible)
 {

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 8a755ca34705f97cee4176cb88bc6259ed8c5fe8..1c32772a88b50547fd3438029eb473bd10f13399 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -195,7 +195,7 @@ static void
 __gnttab_map_grant_ref(
     struct gnttab_map_grant_ref *op)
 {
-    struct domain *ld, *rd;
+    struct domain *ld, *rd, *owner;
     struct vcpu   *led;
     int            handle;
     unsigned long  frame = 0, nr_gets = 0;
@@ -336,8 +336,13 @@ __gnttab_map_grant_ref(
 
     spin_unlock(&rd->grant_table->lock);
 
-    if ( is_iomem_page(frame) )
+    if ( !mfn_valid(frame) ||
+         (owner = page_get_owner_and_reference(mfn_to_page(frame))) == dom_io )
     {
+        /* Only needed the reference to confirm dom_io ownership. */
+        if ( mfn_valid(frame) )
+            put_page(mfn_to_page(frame));
+
         if ( !iomem_access_permitted(rd, frame, frame) )
         {
             gdprintk(XENLOG_WARNING,
@@ -352,20 +357,11 @@ __gnttab_map_grant_ref(
         if ( rc != GNTST_okay )
             goto undo_out;
     }
-    else
+    else if ( owner == rd )
     {
-        if ( unlikely(!mfn_valid(frame)) ||
-             unlikely(!(gnttab_host_mapping_get_page_type(op, ld, rd) ?
-                        get_page_and_type(mfn_to_page(frame), rd,
-                                          PGT_writable_page) :
-                        get_page(mfn_to_page(frame), rd))) )
-        {
-            if ( !rd->is_dying )
-                gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
-                         frame);
-            rc = GNTST_general_error;
-            goto undo_out;
-        }
+        if ( gnttab_host_mapping_get_page_type(op, ld, rd) &&
+             !get_page_type(mfn_to_page(frame), PGT_writable_page) )
+            goto could_not_pin;
 
         nr_gets++;
         if ( op->flags & GNTMAP_host_map )
@@ -383,6 +379,17 @@ __gnttab_map_grant_ref(
             }
         }
     }
+    else
+    {
+    could_not_pin:
+        if ( !rd->is_dying )
+            gdprintk(XENLOG_WARNING, "Could not pin grant frame %lx\n",
+                     frame);
+        if ( owner != NULL )
+            put_page(mfn_to_page(frame));
+        rc = GNTST_general_error;
+        goto undo_out;
+    }
 
     if ( need_iommu(ld) &&
          !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&

diff --git a/xen/common/xencomm.c b/xen/common/xencomm.c
index c1e32bb0de2b97d94d9cdfd76ba575936d607870..0bf00a78262fa570c2f90232b75427296f72c577 100644 (file)
--- a/xen/common/xencomm.c
+++ b/xen/common/xencomm.c
@@ -51,24 +51,16 @@ xencomm_get_page(unsigned long paddr, struct page_info **page)
         return -EFAULT;
         
     *page = maddr_to_page(maddr);
-    if ( get_page(*page, current->domain) == 0 )
+    if ( !get_page(*page, current->domain) == 0 )
     {
-        if ( page_get_owner(*page) != current->domain )
-        {
-            /*
-             * This page might be a page granted by another domain, or
-             * this page is freed with decrease reservation hypercall at
-             * the same time.
-             */
-            gdprintk(XENLOG_WARNING,
-                     "bad page is passed. paddr 0x%lx maddr 0x%lx\n",
-                     paddr, maddr);
-            return -EFAULT;
-        }
-
-        /* Try again. */
-        cpu_relax();
-        return -EAGAIN;
+        /*
+         * This page might be a page granted by another domain, or this page 
+         * is freed with decrease reservation hypercall at the same time.
+         */
+        gdprintk(XENLOG_WARNING,
+                 "bad page is passed. paddr 0x%lx maddr 0x%lx\n",
+                 paddr, maddr);
+        return -EFAULT;
     }
 
     return 0;

diff --git a/xen/include/asm-ia64/mm.h b/xen/include/asm-ia64/mm.h
index 103628476c8dcbc23d3fa924c5f3c540b589e2d0..9cadc21ba4bca8b004ec070a583f75e95601a356 100644 (file)
--- a/xen/include/asm-ia64/mm.h
+++ b/xen/include/asm-ia64/mm.h
@@ -200,9 +200,7 @@ static inline void put_page(struct page_info *page)
         free_domheap_page(page);
 }
 
-/* count_info and ownership are checked atomically. */
-static inline int get_page(struct page_info *page,
-                           struct domain *domain)
+static inline page_get_owner_and_reference(struct page_info *page)
 {
     unsigned long x, y = page->count_info;
 
@@ -210,12 +208,21 @@ static inline int get_page(struct page_info *page,
         x = y;
         if (unlikely((x & PGC_count_mask) == 0) ||  /* Not allocated? */
             unlikely(((x + 1) & PGC_count_mask) == 0) ) {/* Count overflow? */
-            goto fail;
+            return NULL;
         }
         y = cmpxchg_acq(&page->count_info, x, x + 1);
     } while (unlikely(y != x));
 
-    if (likely(page_get_owner(page) == domain))
+    return page_get_owner(page);
+}
+
+/* count_info and ownership are checked atomically. */
+static inline int get_page(struct page_info *page,
+                           struct domain *domain)
+{
+    struct domain *owner = page_get_owner_and_reference(page);
+
+    if (likely(owner == domain))
         return 1;
 
     put_page(page);
@@ -224,7 +231,7 @@ fail:
     gdprintk(XENLOG_INFO,
              "Error pfn %lx: rd=%p, od=%p, caf=%016lx, taf=%" PRtype_info "\n",
              page_to_mfn(page), domain,
-             page_get_owner(page), y, page->u.inuse.type_info);
+             owner, page->count_info, page->u.inuse.type_info);
     return 0;
 }
 

diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index cde79e948ea76a1d65f43d853d97de4a666b2d4f..7772e333cba5a55644978e2550a95fd6fb5c6e20 100644 (file)
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -258,6 +258,7 @@ void cleanup_page_cacheattr(struct page_info *page);
 
 int is_iomem_page(unsigned long mfn);
 
+struct domain *page_get_owner_and_reference(struct page_info *page);
 void put_page(struct page_info *page);
 int  get_page(struct page_info *page, struct domain *domain);
 void put_page_type(struct page_info *page);